Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split the bank snapshot construction from file PR. These are the non-essential helper changes #29311

Closed

Conversation

xiangzhu70
Copy link
Contributor

@xiangzhu70 xiangzhu70 commented Dec 17, 2022

Problem

This is the first step to split the PR 28745 #28745

Summary of Changes

Move the non-essential changes into this PR.
They are:
Some helper functions;
Debug message, error message changes;
Additional checking, such as buildins;
Move the async remove function from validator to snapshot_utils.rs, because it is a util function, and it is easier to be used by the snapshot accounts operations.

Fixes #

@xiangzhu70 xiangzhu70 self-assigned this Dec 17, 2022
@xiangzhu70 xiangzhu70 marked this pull request as ready for review December 17, 2022 06:01
Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good and will help the #28745 be simpler to review. Some nits & clarifying comments.

core/src/validator.rs Show resolved Hide resolved
runtime/src/accounts_db.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/serde_snapshot.rs Outdated Show resolved Hide resolved
@brooksprumo
Copy link
Contributor

I would recommend splitting up this PR further so that each commit represents a single logical change. From looking over this, I see the following distinct changes:

  • Moving move_and_async_delete_path() (and friends) to snapshot_utils
  • Changing behavior of cleanup_accounts_paths()
  • Adding more to the debug log in try_recycle_store()
  • Adding more to the error message in AppendVec::new_from_file()
  • Adding more to the warning in add_builtin_account()
  • Using checked-math to make reconstruct_accountsdb_from_fields() safer
  • Fixing the doc-comment on packager_thread_niceness_adj in SnapshotConfig
  • Refactor out snapshot_write_version_file()
  • Update log in bank_fields_from_snapshots()
  • Update log in rebuild_bank_from_snapshots()
  • Refactor regular expressions in get_snapshot_file_kind()
  • Making get_snapshot_file_kind() public and thus making SnapshotFileKind public
  • Adding debug log in validator::main()

This is quite a list! And it's a testament to how much work you've already put into this feature, @xiangzhu70. I do think it'll be beneficial to have smaller PRs. Ideally the PR's title is a single tangible thing that the PR implements/changes vs a collection of non-essential "helper" changes.

Hopefully many of the debugging/logging changes probably would go in with another PR that uses those new logs. Each distinct code refactoring would benefit from its own PR; this makes it easier to review and really know what is and isn't getting updated.

I imagine this may feel like a lot of extra work; that's how I felt when I started on this codebase as well. What I've found is that over time these smaller PRs are much faster. Reviews get done much faster, far fewer bugs get introduced, and writing out PR descriptions helps me think through the intent of the PR as well (and then the reviewers get that context too!).

@apfitzge
Copy link
Contributor

I imagine this may feel like a lot of extra work; that's how I felt when I started on this codebase as well. What I've found is that over time these smaller PRs are much faster. Reviews get done much faster, far fewer bugs get introduced, and writing out PR descriptions helps me think through the intent of the PR as well (and then the reviewers get that context too!).

I second the thought that several smaller PRs is easier to review and generally will go faster overall 😄
It additionally has the benefit of making introduced bugs easier to track down the exact breaking change using git bisect
I was split on pushing this here, but with @brooksprumo also expressing this I think it makes sense to split it out.

@xiangzhu70
Copy link
Contributor Author

For the essential major changes, splitting helps isolate the impact of potential problems for bisecting. This PR only has misc logging/helper logic which have no impact to the existing behaviors, is meant to reduce the size of the essential PRs. I thought it should be safe. OK, I will split it further. It indeed will take quite much work. I will separate them by functionality and generate different PRs. Closing this PR now.

@xiangzhu70 xiangzhu70 closed this Dec 23, 2022
Moneymustbemade707

This comment was marked as spam.

@mergify mergify bot added community Community contribution need:merge-assist labels Jan 1, 2025
@solana-labs solana-labs locked as resolved and limited conversation to collaborators Jan 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community Community contribution need:merge-assist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants